Conversation
this test would not compile because TA inherited compile OpenMP dependence from blaspp (?) .. and OpenMP is not quite C++.
…sumers (incl. SQ tests) on TA/BTAS ... and to model the module structure described MS1.
There was a problem hiding this comment.
Pull request overview
This PR refactors the SeQuant CMake build system from a monolithic structure to a modular, layered architecture. The changes partition the codebase into logical components (symbolic core, evaluation framework, export/codegen, MBPT domain, and backend-specific modules), improve dependency management, and update installation/configuration logic. Additionally, optimization code is relocated from SeQuant/core/optimize.* to SeQuant/core/eval/optimize.*, with all includes updated throughout the codebase, and several typos are corrected.
Changes:
- Introduced modular CMake targets:
SeQuant-symb,SeQuant-eval,SeQuant-eval-ta,SeQuant-eval-btas,SeQuant-export,SeQuant-core, andSeQuant-mbpt, with an umbrellaSeQuantINTERFACE target for backwards compatibility - Reorganized optimization code from
core/optimizetocore/eval/optimizedirectory structure with corresponding header path updates across the entire codebase - Restructured unit tests into separate OBJECT libraries aligned with the new modular targets, improving build organization and compilation efficiency
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| CMakeLists.txt | Defines new modular library targets with explicit source lists and dependencies; updates installation and IWYU configuration |
| cmake/sequant-config.cmake.in | Updates target verification logic to check for all new modular targets |
| tests/unit/CMakeLists.txt | Restructures tests into OBJECT libraries per module; maintains conditional backend compilation |
| utilities/external-interface/*.cpp | Updates includes from core/optimize to core/eval/optimize |
| tests/unit/test_*.cpp | Updates includes to reflect new header locations |
| tests/integration/eval/calc_info.hpp | Updates include path for optimize header |
| SeQuant/core/eval/optimize/*.{hpp,cpp} | Adds new header files for modularized optimization code |
| SeQuant/core/expressions/result_expr.cpp | Removes unnecessary optimize.hpp include |
| SeQuant/domain/mbpt/spin.cpp | Updates include path for optimize functionality |
| tests/unit/gwt.hpp | Changes from structured binding to explicit reference extraction (compatibility/style) |
| doc/user/guide/operator.rst | Clarifies operator label case convention |
| SeQuant/core/eval/optimize.hpp | Corrects spelling: "aproximate_size()" → "approximate_size()" |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Krzmbrzl
left a comment
Krzmbrzl
left a comment
There was a problem hiding this comment.
I know that a lot of the "could be private" comments are not inherently new due to changes made in this PR but I feel like the strive for a more modularized setup makes these kinds of things much more important.
CMakeLists.txt
Outdated
| target_link_libraries(SeQuant INTERFACE SeQuant-eval-btas) | ||
| endif() | ||
| if (SEQUANT_EVAL_TRACE) | ||
| target_compile_definitions(SeQuant INTERFACE SEQUANT_EVAL_TRACE=1) |
There was a problem hiding this comment.
Shouldn't this be defined on the eval module 👀
tests/unit/CMakeLists.txt
Outdated
| Catch2::Catch2 | ||
| dtl::dtl | ||
| Boost::boost | ||
| Eigen3::Eigen) |
There was a problem hiding this comment.
I believe Eigen is only needed for the export validation tests, which aren't part of this object library.
There was a problem hiding this comment.
Dependency handling could be more scoped - currently it seems all dependencies are added to all module tests.
- introduced `optimize` module - introduced sequant_add_library to cleanup module creation - cleanup dependencies
|
@Krzmbrzl defining proper API boundaries is too much work for now, I've done a bit, but it feels like bikeshedding. Enough for now. |
evaleev
force-pushed
the
evaleev/feature/target-reord
branch
from
51c66ac to
e7cd9f0
Compare
evaleev
force-pushed
the
evaleev/feature/target-reord
branch
from
e7cd9f0 to
0992461
Compare
3 participants
along the lines of #391, prompted by devious OpenMP compilation injection into TA breaking one of the tests. This includes not only logical reorganization into proto-C++-modules, but some physical layout adjustments to more cleanly separate symbolic core from eval from export. @Krzmbrzl have a peek.
Copilot description follows.
This pull request significantly restructures the CMake build system for the SeQuant project, introducing a modular, layered approach to library targets. The changes partition the codebase into logical components (symbolic, evaluation, export, MBPT domain, etc.), improve dependency management, and update installation and configuration logic accordingly. Additionally, some header inclusions and typos are fixed, and documentation is updated for clarity.
CMake build system modularization and improvements:
CMakeLists.txtto define separate CMake targets for each logical layer:SeQuant-symb(symbolic core),SeQuant-eval(evaluation framework),SeQuant-eval-ta(TiledArray backend),SeQuant-eval-btas(BTAS backend),SeQuant-export(export/codegen),SeQuant-core(version info, links symbolic+export), andSeQuant-mbpt(MBPT domain code). An umbrellaSeQuantINTERFACE target is provided for backwards compatibility. [1] [2] [3] [4] [5] [6]sequant-config.cmake.in.include-what-you-use(IWYU) by applying it to all new targets, not just the monolithic one.Code organization and header updates:
SeQuant/core/optimize.*toSeQuant/core/eval/optimize.*and updated all includes accordingly. [1] [2] [3] [4] [5]aproximate_size()toapproximate_size()inSeQuant/core/eval/optimize.hpp. [1] [2] [3]Documentation:
doc/user/guide/operator.rst("T" or "Λ" → "t" or "λ").